-
Notifications
You must be signed in to change notification settings - Fork 3
gh-45 : fix git-sh-setup missing problem #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe main script now sets Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
git-artifact(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: MadsNielsen
Repo: Praqma/git-artifact PR: 0
File: :0-0
Timestamp: 2025-08-28T13:26:41.036Z
Learning: In git-artifact bash script, the fetch-co|push command validation logic that requires "at least one of -t|--tag <tag> or -b|--branch" was already present before PR #40 and was not introduced by that PR. PR #40 only removed git-sh-setup dependency and replaced require_work_tree with need_workspace().
📚 Learning: 2025-08-28T13:26:41.036Z
Learnt from: MadsNielsen
Repo: Praqma/git-artifact PR: 0
File: :0-0
Timestamp: 2025-08-28T13:26:41.036Z
Learning: In git-artifact bash script, the fetch-co|push command validation logic that requires "at least one of -t|--tag <tag> or -b|--branch" was already present before PR #40 and was not introduced by that PR. PR #40 only removed git-sh-setup dependency and replaced require_work_tree with need_workspace().
Applied to files:
git-artifact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses issue gh-45 by conditionalizing the which git-sh-setup command to only run during debug mode, preventing issues when git-sh-setup is not available.
Key Changes:
- Wrapped the
which git-sh-setuptest command in a debug-only conditional block
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
git-artifact
Outdated
| shift | ||
|
|
||
| which git-sh-setup | ||
| [[ ${debug:-} == true ]] && { |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition has two issues: (1) it references debug but the debug flag is stored in arg_debug throughout the file (set to 1 on line 526), and (2) it compares against the string "true" but arg_debug is set to 1, not "true". As a result, this condition will never evaluate to true and the which command will never execute. Consider using a condition like test -n "${arg_debug:-}" which is the pattern used by the debug function on line 120.
34225fd to
288f431
Compare
288f431 to
cea29dc
Compare
Only test during debug
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.